Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

renaming rfc4122 functions #754

Closed
wants to merge 2 commits into from
Closed

Conversation

Mikopet
Copy link
Contributor

@Mikopet Mikopet commented May 8, 2024

Hey!

My second contribution is a small rename of certain obsolete function names. (namely, contains rfc4122)

The changes are cosmetic, does not carry real value...
... but it has some documental changes and additions regarding deprecation messages.


Majority of the functions were private, but there was also a few public ones. This latter ones got proxy functions in place to not break BC, with the proper deprecation warning.

During my testing it works as expected... also, I took the liberty and put the next version number into the deprecation messages, indicating there will be a patch version published.

When we will have version 2.0, we could clean up deprecated features.

Preferably should be merged after #753

@Mikopet
Copy link
Contributor Author

Mikopet commented May 9, 2024

as per CONTRIBUTING.md states, I hereby pinging one of you. ( @kinggoesgaming )

Also suggesting checking on my other PR #753

Copy link
Member

@KodrAus KodrAus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR @Mikopet. I can see the motivation for doing this, especially since the old RFC is now obsolete in favour of 9562. I think the name rfc on its own isn't specific enough to tell what it should be so we could make it rfc9562 instead. The old RFC got almost 20 years of mileage out of it so I don't think we're at a big risk of needing to do this a second time anytime soon.

Having said that, I'm not fully convinced we should do this. I personally find these kinds of pure-renaming refactorings mostly just a nuisance as a user. I think it's something to keep up our sleeves though if we find a better API in the future to push users to.

@Mikopet
Copy link
Contributor Author

Mikopet commented May 13, 2024

I think the name rfc on its own isn't specific enough to tell what it should be so we could make it rfc9562 instead. The old RFC got almost 20 years of mileage out of it so I don't think we're at a big risk of needing to do this a second time anytime soon.

I agree, sort of. name rfc is kinda meaningless unless somebody is familiar with it. However, the name rfc9562 is kinda the same, just feels like code smell to me. Perhaps a name like gregorian would make sense. (as the RFC in fact call it that way multiple times)

I'm not fully convinced we should do this. [...] just a nuisance as a user. I think it's something to keep up our sleeves though if we find a better API in the future to push users to.

No disagree here, this PR indeed is some bikeshedding. More like an attempt to push the crate in the direction for some cleanup efforts.. as it is a little bit lorn... yet, it is very much on the important side of all crates. (top 100)

I do have some new ideas for a new API, would mean a major release. I will share it on the relevant PR soon (when the PoC is working as expected)


so, back to this PR. What do you think about a rename to from_gregorian_timestamp()/gregorian_to_unix()/etc?
If you are up for it, I adjust the PR... if not, just close it. no hard feelings :)

@KodrAus
Copy link
Member

KodrAus commented Jun 17, 2024

I think we can revisit this after I finish #755, it ends up needing to add to the timestamp API to support the more sophisticated counter requirements in v7 UUIDs.

@KodrAus
Copy link
Member

KodrAus commented Jun 23, 2024

#755 is merged now. I think if we rename these, we could possibly use the name gregorian_timestamp rather than rfc_timestamp to distinguish the 100ns ticks since October 15, 1582, and the 1ns ticks since Janurary 1, 1970.

I don't think this needs to block our next release, because it doesn't add any new methods we'd want to deprecate.

@Mikopet Mikopet force-pushed the rename_rfc_fns branch 2 times, most recently from 6f6cafb to 123daaa Compare June 24, 2024 15:58
@Mikopet
Copy link
Contributor Author

Mikopet commented Jun 24, 2024

Renamed the rfcs to gregorian.

Also I attempted to rename the Variant field, with backward compatibility. It adds a slight plus complexity, but will disappear when we delete deprecated items.

It may not be fully super BC... but I guess for that User has to do some very niche edge case what they probably should not do any ways.

https://github.com/uuid-rs/uuid/pull/754/files#diff-b1a35a68f14e696205874893c07fd24fdb88882b47c23cc0e0c80a30c7d53759R350

Anyways, feel free to cherry pick... or just close if it's not needed. (deprecation warnings points to future version 1.9.1)

@KodrAus
Copy link
Member

KodrAus commented Jun 24, 2024

Thanks @Mikopet. I think the name Variant::RFC suffers from the same issues as rfc in function names but I think we should be ok to leave Variant as-is. It's already pretty niche. We'll deprecate the function names in the next minor release. Would you be happy to change those to point to 1.10.0?

@Mikopet
Copy link
Contributor Author

Mikopet commented Jun 25, 2024

I think the name Variant::RFC suffers from the same issues as rfc in function names

Indeed, but not that much. My reasoning is: while the RFC mentions "Gregorian" many times (so they call it by name), the variant field just says: The variant specified in this document.

should be ok to leave Variant as-is. It's already pretty niche.

I agree on that, it is indeed very niche. This is more likely a thought experiment and an attempt to put this things in order in a compatible way.

To be honest, I don't really like the current Variant/Version enums. They are not representating the things written in the RFC very well. I would suggest a more nearby approach to it.

I don't have ready implementation for it, but some idea I do have. I'm happy to jump on it, if you think it could be a good direction:

  • Having one enum for both, something like Field/Parameter or similar.
  • This enum would have two fields, exhaustive: Version(u4), Variant(u4)
  • When we do override, instead of .with_version()/.with_variant() we simply use .with(Parameter)
  • .set_*() same. perhaps the name of that function also could change to terminology in the RFC.
  • I'm unsure about the .get_*(), but perhaps it could return with a tuple

Anyhow, I updated the commits!

@KodrAus
Copy link
Member

KodrAus commented Jul 8, 2024

Thanks for working on this @Mikopet! I've cherry-picked your first commit into #765, which I plan to release as 1.10.0.

@KodrAus KodrAus closed this Jul 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants